ROS2: boostrap repo and convert gazebo_msgs #769

kev-the-dev · 2018-07-13T03:25:40Z

Convert the messages in gazebo_msgs to ROS2 compliant message files (replaces gazebo_msgs: converted messages to ros2 format. #735)
Move all remaining code to .ros1_unported, which has IGNORE files to be ignored by ament/colcon. This will ensure the package builds and individual files will be moved back to into the root directory as they are ported over

chapulina

Thanks for getting the ball rolling! I left some preliminary comments, mostly about documentation and styling.

chapulina · 2018-07-17T01:10:45Z

gazebo_msgs/package.xml

  </description>

-  <maintainer email="jrivero@osrfoundation.org">Jose Luis Rivero</maintainer>
+  <author>John Hsu</author>
+  <maintainer email="ecorbellini@ekumenlabs.com">Ernesto Corbellini</maintainer>


Are the author and maintainer changes on purpose?

Oops, will fix

chapulina · 2018-07-17T01:11:42Z

gazebo_msgs/msg/ODEJointProperties.msg

-float64[] hiStop              # joint limit
-float64[] loStop              # joint limit
+float64[] hi_stop              # joint limit
+float64[] lo_stop              # joint limit


nitpick: align the comments on the right

chapulina · 2018-07-17T01:12:13Z

gazebo_msgs/srv/ApplyBodyWrench.srv

                                          # if start_time is not specified, or
                                          # start_time < current time, start as soon as possible
-duration duration                         # optional duration of wrench application time (seconds)
+builtin_interfaces/Duration duration                         # optional duration of wrench application time (seconds)


nit: comment alignment

chapulina · 2018-07-17T01:15:54Z

gazebo_plugins/src/node_example.cpp

@@ -0,0 +1,33 @@
+#include <gazebo_plugins/node_example.hpp>


Missing copyright header here and on several other files

chapulina · 2018-07-17T01:19:31Z

gazebo_ros/include/gazebo_ros/executor.hpp

+namespace gazebo_ros
+{
+
+/// Executor run in a seperate thread to handle events from all #Node instances


Could you use doxygen syntax on the whole documentation? i.e. /// \brief

\brief is implied with the first line of a doxygen block so my personal style preference is to omit it (it will still generate correctly)

Interesting, I didn't know. I'm good leaving it out then.

chapulina · 2018-07-17T01:23:07Z

gazebo_ros/include/gazebo_ros/node.hpp

+      NotInitializedException();
+  };
+
+  typedef std::shared_ptr<Node> SharedPtr;


How about making this type more explicit? Such as NodeSharedPtr? That should make the API clearer and we don't end up reserving a very general "keyword".

This is the convention used in other parts of the ROS2 codebase so far. The typedef is within the class, so it is accessed as gazebo_ros::Node::SharedPtr

Ahh got it, good call!

chapulina · 2018-07-17T01:32:12Z

gazebo_ros/include/gazebo_ros/node.hpp

+   */
+  static SharedPtr Create(const std::string& node_name);
+
+  /**


I realize the ROS 1 code mixes /// with /**, the ROS 2 developer guide accepts either and they're both supported by Doxygen.

I think there's value in being consistent within a single repository though, so I think we should define now which one to use. I don't have a preference, so I'll leave up to you or anyone with strong opinions.

I agree it should be consistent. Looks like some other ros2 code always starts with /// and adds /** */ if more lines are required

Oh interesting, that looks weird to me on a first look, I wonder how that gets rendered and what's the rationale behind it.

chapulina · 2018-07-17T01:32:30Z

gazebo_ros/include/gazebo_ros/node.hpp

+
+  /// Initialize ROS with the command line arguments.
+  /// Must be called ONCE before ande #Node instances are created
+  static void InitROS(int argc, char** argv);


Document input params

chapulina · 2018-07-17T01:33:26Z

gazebo_ros/package.xml

  <name>gazebo_ros</name>
  <version>2.8.4</version>
  <description>
-    Provides ROS plugins that offer message and service publishers for interfacing with <a href="http://gazebosim.org">Gazebo</a> through ROS.
-    Formally simulator_gazebo/gazebo
+    Utilities to interface with Gazebo <a href="http://gazebosim.org">Gazebo</a> through ROS.


Gazebo is written twice

chapulina · 2018-07-17T01:34:15Z

gazebo_ros/package.xml

-  <!--
-    Need to use gazebo_dev since run script needs pkg-config
-    See: https://github.com/ros-simulation/gazebo_ros_pkgs/issues/323 for more info
-  -->
  <exec_depend>gazebo_dev</exec_depend>


gazebo_dev is already listed above

chapulina · 2018-07-17T20:15:12Z

gazebo_msgs/CMakeLists.txt

+  "srv/SetJointTrajectory.srv"
+  "srv/GetLightProperties.srv"
+  "srv/SetLightProperties.srv"
+)


How do you feel about alphabetizing this list? The messages are already ok

chapulina · 2018-07-17T20:17:07Z

How about splitting this PR so it's easier to review? It could start with just porting the messages and moving all unported code to the separate folder.

chapulina · 2018-07-17T20:25:50Z

gazebo_dev/package.xml

@@ -1,5 +1,6 @@
 <?xml version="1.0"?>
-<package format="2">
+<?xml-model href="http://download.ros.org/schema/package_format3.xsd" schematypens="http://www.w3.org/2001/XMLSchema"?>
+<package format="3">


Can't we keep format 2?

We could, but format 3 offers some advantages and will likely be supported for longer. It seems to be the default for new ROS2 packages

In general, I think it's best not to upgrade unless there's an explicit reason to, so we keep compatibility with older tools.

It doesn't look to me that we're using anything specific to version 3 here, so I don't see a need for the move. But it doesn't look like it's breaking anything either, so we can leave it as 3 for as long as we don't see a problem with it.

kev-the-dev · 2018-07-17T20:41:21Z

The PR splitting sounds good, time to put on my git hat

kev-the-dev · 2018-07-17T21:26:49Z

Split out everything besides the file moves and message conversions. The rest is on other branches I can PR when this one is in

chapulina · 2018-07-17T21:51:02Z

gazebo_msgs/package.xml


+  <build_depend>rosidl_default_generators</build_depend>


Should this be buildtool_depend?

Switched, that seems like the right way to do it

chapulina · 2018-07-17T22:01:09Z

Thanks, @ironmig ! This PR looks good to me, I just have one small comment left about buildtool_depend

chapulina

Looks good to me!

kev-the-dev mentioned this pull request Jul 13, 2018

Port gazebo_ros_pkgs to ROS 2.0 #512

Closed

chapulina reviewed Jul 17, 2018

View reviewed changes

kev-the-dev force-pushed the ros2-devel branch from 3fb786f to 84b195c Compare July 17, 2018 19:12

chapulina reviewed Jul 17, 2018

View reviewed changes

kev-the-dev force-pushed the ros2-devel branch from 84b195c to f491f8d Compare July 17, 2018 20:39

Ernesto Corbellini and others added 2 commits July 17, 2018 13:50

Converted gazebo_msgs to ros2.

8d4ca31

Move all unported files to .ros1_unported

37726f6

kev-the-dev force-pushed the ros2-devel branch from f491f8d to 37726f6 Compare July 17, 2018 20:57

kev-the-dev changed the title ~~Initial work for ROS2 support~~ ROS2: boostrap repo and convert gazebo_msgs Jul 17, 2018

kev-the-dev mentioned this pull request Jul 17, 2018

gazebo_msgs: converted messages to ros2 format. #735

Closed

chapulina reviewed Jul 17, 2018

View reviewed changes

Switch rosidl_default_generators to buildtool_depend

aa446d0

chapulina approved these changes Jul 17, 2018

View reviewed changes

kev-the-dev merged commit 03ec199 into ros-simulation:ros2 Jul 17, 2018

dhood added the ros2 label Aug 17, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ROS2: boostrap repo and convert gazebo_msgs #769

ROS2: boostrap repo and convert gazebo_msgs #769

kev-the-dev commented Jul 13, 2018 •

edited

Loading

chapulina left a comment

chapulina Jul 17, 2018

kev-the-dev Jul 17, 2018

chapulina Jul 17, 2018

chapulina Jul 17, 2018

chapulina Jul 17, 2018

chapulina Jul 17, 2018

kev-the-dev Jul 17, 2018

chapulina Jul 17, 2018

chapulina Jul 17, 2018

kev-the-dev Jul 17, 2018

chapulina Jul 17, 2018

chapulina Jul 17, 2018

kev-the-dev Jul 17, 2018

chapulina Jul 17, 2018

chapulina Jul 17, 2018

chapulina Jul 17, 2018

chapulina Jul 17, 2018

chapulina Jul 17, 2018

chapulina commented Jul 17, 2018

chapulina Jul 17, 2018

kev-the-dev Jul 17, 2018

chapulina Jul 17, 2018

kev-the-dev commented Jul 17, 2018

kev-the-dev commented Jul 17, 2018

chapulina Jul 17, 2018

kev-the-dev Jul 17, 2018

chapulina commented Jul 17, 2018

chapulina left a comment

ROS2: boostrap repo and convert gazebo_msgs #769

ROS2: boostrap repo and convert gazebo_msgs #769

Conversation

kev-the-dev commented Jul 13, 2018 • edited Loading

chapulina left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chapulina commented Jul 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kev-the-dev commented Jul 17, 2018

kev-the-dev commented Jul 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chapulina commented Jul 17, 2018

chapulina left a comment

Choose a reason for hiding this comment

kev-the-dev commented Jul 13, 2018 •

edited

Loading